Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(partitions): Small multiples legends #1094

Merged
merged 10 commits into from
Mar 30, 2021
Merged

feat(partitions): Small multiples legends #1094

merged 10 commits into from
Mar 30, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Mar 29, 2021 •
edited
Loading

Summary

Flat legends (now sorted alphabetically; cursor hovering over Minerals - green - to highlight):
image

Hierarchical legends:
image

Checklist

Delete any items that are not applicable to this PR.

  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials

@monfera monfera added :partition Partition/PieChart/Donut/Sunburst/Treemap chart related :small multiples Small multiples/trellising related issues labels Mar 29, 2021
@monfera monfera requested a review from markov00 March 29, 2021 19:49
markov00
markov00 reviewed Mar 30, 2021
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've few doubts on what was changed here:
https://github.com/elastic/elastic-charts/blob/9aa1d202b6ccc55dc3169f2a100a3b9ba3943761/integration/tests/__image_snapshots__/legend-stories-test-ts-legend-stories-should-have-the-same-order-as-nested-with-no-indent-even-if-there-are-repeated-labels-1-snap.png

I've added the legendStrategy to that story and it looks a bit wrong: it doesn't work correctly with all the strategies. Is that a wanted behaviour?

@codecov-io
Copy link

Codecov Report

Merging #1094 (d899405) into master (2a8bd89) will increase coverage by 0.00%.
The diff coverage is 43.75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1094   +/-   ##
=======================================
  Coverage   71.71%   71.71%           
=======================================
  Files         383      381    -2     
  Lines       11924    11918    -6     
  Branches     2590     2598    +8     
=======================================
- Hits         8551     8547    -4     
+ Misses       3342     3340    -2     
  Partials       31       31
Flag Coverage Δ
unittests 71.71% <43.75%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/partition_chart/layout/types/viewmodel_types.ts 87.50% <ø> (ø)
.../partition_chart/layout/utils/highlighted_geoms.ts 19.04% <0.00%> (ø)
...types/partition_chart/renderer/dom/highlighter.tsx 15.55% <0.00%> (-0.54%) ⬇️
...partition_chart/renderer/dom/highlighter_hover.tsx 47.05% <0.00%> (-2.95%) ⬇️
...artition_chart/renderer/dom/highlighter_legend.tsx 44.44% <0.00%> (-8.89%) ⬇️
...on_chart/state/selectors/get_highlighted_shapes.ts 63.63% <0.00%> (ø)
src/specs/settings.tsx 90.32% <ø> (ø)
...chart_types/partition_chart/layout/utils/legend.ts 76.92% <68.75%> (-3.08%) ⬇️
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 85.08% <100.00%> (-0.71%) ⬇️
...ypes/partition_chart/state/selectors/geometries.ts 76.97% <100.00%> (+2.37%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58e6b5f...d899405. Read the comment docs.

markov00
markov00 approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, agreed on the new partition flat legend alphabetical order.
Lens uses the flat version with only the root layer or the hierarchical version with all layers.

@monfera monfera merged commit c39d113 into elastic:master Mar 30, 2021
@markov00 markov00 mentioned this pull request Mar 31, 2021
Closed
3 tasks
github-actions bot pushed a commit that referenced this pull request Mar 31, 2021
@semantic-release-bot
chore(release): 27.0.0 [skip ci]
27035ea 
# [27.0.0](v26.1.0...v27.0.0) (2021-03-31)

### Features

* **partitions:** Small multiples legends ([#1094](#1094)) ([c39d113](c39d113))

### BREAKING CHANGES

* **partitions:** the `flatLegend` (true) option yields alphabetical, formatted name based sorting for unique name/color occurrences, to make it easy for the user to look up names in the legend as it's alphabetically sorted
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 27.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Mar 31, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 27.0.0 [skip ci]
02cab27 
# [27.0.0](elastic/elastic-charts@v26.1.0...v27.0.0) (2021-03-31)

### Features

* **partitions:** Small multiples legends ([opensearch-project#1094](elastic/elastic-charts#1094)) ([02e45d6](elastic/elastic-charts@02e45d6))

### BREAKING CHANGES

* **partitions:** the `flatLegend` (true) option yields alphabetical, formatted name based sorting for unique name/color occurrences, to make it easy for the user to look up names in the legend as it's alphabetically sorted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
:partition Partition/PieChart/Donut/Sunburst/Treemap chart related released Issue released publicly :small multiples Small multiples/trellising related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@monfera @codecov-io @nickofthyme @markov00